fix(ACI): Migrate fallthroughType to fallthrough_type #103764
Conversation
github-actions
bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
src/sentry/workflow_engine/migrations/0104_action_data_fallthrough_type.py
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
src/sentry/workflow_engine/migrations/0104_action_data_fallthrough_type.py
Outdated
Show resolved
Hide resolved
src/sentry/workflow_engine/migrations/0103_action_data_fallthrough_type.py
Outdated
Show resolved
Hide resolved
|
This PR has a migration; here is the generated SQL for for --
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL |
src/sentry/workflow_engine/migrations/0103_action_data_fallthrough_type.py
Outdated
Show resolved
Hide resolved
src/sentry/workflow_engine/migrations/0104_action_data_fallthrough_type.py
Show resolved
Hide resolved
| # is a schema change, it's completely safe to run the operation after the code has deployed. | ||
| # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment | ||
|
|
||
| is_post_deployment = False |
There was a problem hiding this comment.
Do we want this to be a post-deploy migration?
There was a problem hiding this comment.
Updated since this table has 2,937,063 rows
There was a problem hiding this comment.
There are enough rows in this table in production that it should be post_deploy.
If "suggested assignees" are selected when creating an action that uses the fallthrough type the schema was incorrectly expecting it to be camel case instead of snake case which prevented the action from being created. This updates that and adds a test case around that particular action. For now we must support both types until we can run a migration, after which I can go back and remove support for the incorrect camel case type. Fixes https://getsentry.atlassian.net/browse/ACI-504 Related migration #103764
ceorourke
force-pushed
the
ceorourke/migrate-action-fallthrough-type
branch
from
1d9eafc to
9202d69
Compare
| # Here are some things that make sense to mark as post deployment: | ||
| # - Large data migrations. Typically we want these to be run manually so that they can be | ||
| # monitored and not block the deploy for a long period of time while they run. | ||
| # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to | ||
| # run this outside deployments so that we don't block them. Note that while adding an index | ||
| # is a schema change, it's completely safe to run the operation after the code has deployed. | ||
| # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment | ||
|
|
||
| is_post_deployment = True | ||
|
|
There was a problem hiding this comment.
Bug: The EmailDataBlob dataclass instantiation will fail after migration due to unexpected keyword arguments from action_data.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
After migration 0104_action_data_fallthrough_type.py runs, the EmailIssueAlertHandler.get_additional_fields() method will attempt to instantiate EmailDataBlob using EmailDataBlob(**action_data). The action_data dictionary will contain keys such as targetType, targetIdentifier, uuid, and id in addition to fallthrough_type. Since the EmailDataBlob dataclass only defines fallthrough_type, passing these unexpected keyword arguments will raise a TypeError (e.g., __init__() got an unexpected keyword argument 'targetType'). This will lead to a server crash whenever an email action with target_type == ActionTarget.ISSUE_OWNERS is processed.
💡 Suggested Fix
Modify the EmailIssueAlertHandler.get_additional_fields() method to filter action_data before instantiating EmailDataBlob, ensuring only fallthrough_type is passed. Alternatively, update the EmailDataBlob dataclass to include all relevant fields from action_data.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
src/sentry/workflow_engine/migrations/0104_action_data_fallthrough_type.py#L27-L36
Potential issue: After migration `0104_action_data_fallthrough_type.py` runs, the
`EmailIssueAlertHandler.get_additional_fields()` method will attempt to instantiate
`EmailDataBlob` using `EmailDataBlob(**action_data)`. The `action_data` dictionary will
contain keys such as `targetType`, `targetIdentifier`, `uuid`, and `id` in addition to
`fallthrough_type`. Since the `EmailDataBlob` dataclass only defines `fallthrough_type`,
passing these unexpected keyword arguments will raise a `TypeError` (e.g., `__init__()
got an unexpected keyword argument 'targetType'`). This will lead to a server crash
whenever an email action with `target_type == ActionTarget.ISSUE_OWNERS` is processed.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3286846
There was a problem hiding this comment.
This seems out of scope and kinda unrelated - I updated EmailDataBlob in the other PR to use fallthrough_type instead of fallthroughType https://github.com/getsentry/sentry/blob/master/src/sentry/workflow_engine/typings/notification_action.py#L737-L743, if it's missing other kwargs that's a different problem.
|
This PR has a migration; here is the generated SQL for for --
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL |
Companion PR to #103694 to migrate
fallthroughTypetofallthrough_typeinAction.data. Other PR must be merged first.